-
Notifications
You must be signed in to change notification settings - Fork 278
WIP give ComponentInterface members a pointer to their parent.
#1003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }} -> { | ||
| {% for field in variant.fields() -%} | ||
| {%- if ci.type_contains_object_references(field.type_()) -%} | ||
| {%- if field.contains_object_references() -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a nice improvement in usability for the template logic.
| pub struct Enum<'a> { | ||
| pub(super) parent: &'a ComponentInterface, | ||
| pub(super) descr: &'a EnumDescr, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the Enum struct is now a kind of "fat pointer" thing that contains both a pointer to the data and parent ref to the containing object.
|
|
||
| pub fn variants(&self) -> Vec<&Variant> { | ||
| self.variants.iter().collect() | ||
| pub fn variants(&self) -> Vec<Variant<'_, Self>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, when consuming code lists the variants of the enum, so see that we create a bunch of Variant instances on demand with the appropriate pointers.
|
|
||
| pub fn contains_object_references(&self, ci: &ComponentInterface) -> bool { | ||
| // *sigh* at the clone here, the relationship between a ComponentInterace | ||
| // and its contained types could use a bit of a cleanup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey look, no more clone!
| #[derive(Debug)] | ||
| pub struct Variant<'a, Parent: CINode> { | ||
| pub(super) parent: &'a Parent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this starts to get a bit complicated...the Variant struct may be used as part of an Enum or as part of an Error, so its parent pointer type needs to be generic. The main property that we want from the parent pointer is the ability to get all the way up to the containing ComponentInterface, so I made a trait for that.
An alternative could be to have it point directly to the containing ComponentInterface rather than its containing object, which would make this more of an "owner" pointer than a "parent" pointer. But, I can imagine reasons why a variant might need to be able to inspect the properties of its containing Enum/Error.
| } | ||
| } | ||
|
|
||
| impl<'a, Parent: CINode + 'a> CINode for Variant<'a, Parent> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could imagine generating these impls automatically from some sort of macro.
| pub fn iter_enum_definitions(&self) -> Vec<Enum<'_>> { | ||
| self.enums | ||
| .iter() | ||
| .map(|e| Enum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again we see that we're synthesizing the aggregate pointer thingy on demand.
|
|
||
| // Represents an individual field on a Record. | ||
| #[derive(Debug)] | ||
| pub struct Field<'a, Parent: CINode> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We re-use the Field struct in two places, for enum variants fields and record fields, hence the generic.
Which makes me wonder...I wonder if enum variants could actually contain a wrapped Record in the same way that errors contain a wrapped Enum...
| assert_eq!(record.fields()[0].name(), "field"); | ||
| assert_eq!(record.fields()[0].type_().canonical_name(), "u32"); | ||
| assert!(!record.fields()[0].required); | ||
| assert!(!record.fields()[0].required()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can no longer access the fields directly because they're hidden behind a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually looks simpler than I thought it would be.
It certainly does make the templating a bit easier, so I'm leaning on the side of "worth the complexity cost"
| pub fn contains_unsigned_types(&self, ci: &ComponentInterface) -> bool { | ||
| self.variants().iter().any(|v| { | ||
| v.fields() | ||
| pub fn contains_unsigned_types(&self, _ci: &ComponentInterface) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason to keep _ci here? Is it necessary or just work-in-progress for now?
Here's something I wanted to toy with given recent work on refactoring our code-generation templates, and the desire for each interface member to be able to render itself. The issue is that sometimes we need to know information about the surrounding interface or its types in order to do the code generation, which shows up in awkward calls like:
What would it take to make this be just a plain method call without extra global context? Like so:
In a garbage-collected language, we might conveniently give the
Record,Enumetc objects a reference to their containingComponentInterfaceand be done with it. That's hard to do in the obvious way in Rust, and the usual workarounds like handing out anArc<RefCell<ComponentInterface>>seem extremely verbose/complex for our needs.Here's a sketch of something that might work, although I'm not convinced of the value/complexity tradeoff.
Taking the
Enumstruct as an example, I've split it up into two parts:EnumDescris the data that describes an enum. Instances of this are owned by aComponentInterfacein a normal tree structure without backwards references.Enumis a kind of aggregate transient pointer, combining a reference to anEnumDescrwith a reference to its containingComponentInterface. Instances of this are synthesized on demand when inspecting aComponentInterface.From the consumer's perspective, it calls
ci.get_enum_definition("MyEnum")and receives anEnumstruct, and it can call methods on theEnumstruct to inspect its definition. But these methods all delegate to the underlyingEnumDescrfor the actual data.It gets a little complicated in the details of nested strcuts like
VariantandField, and I'll leave some musings inline.Thoughts?